feat(snap-account-service): add migration logic + keyring v2 support#8732
Conversation
ccharly
force-pushed
the
cc/feat/snap-account-service-keyring-v2-support
branch
from
96269d0 to
43c238b
Compare
ccharly
changed the base branch from
main
to
cc/feat/snap-account-service-account-group-events
ccharly
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8452267. Configure here.
| this.#migratePromise = this.#migrate(); | ||
| } | ||
| return await this.#migratePromise; | ||
| } |
There was a problem hiding this comment.
Failed migration permanently cached, blocking all retries
Medium Severity
#migratePromise caches the very first promise returned by #migrate() and never resets it. If migration fails (e.g., addNewKeyring or removeKeyring rejects), the rejected promise is permanently stored. Every subsequent call to migrate() — including from ensureReady() — returns this same rejected promise with no retry path. This effectively disables all snap account functionality until the app is fully restarted.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8452267. Configure here.
1 participant
Explanation
Add migration logic to migrate from the legacy Snap keyring to the new architecture with 1 Snap keyring v2 per Snaps.
Also enhance
:ensureReadyso consumers can safely call:withKeyringV2to get the associated Snap keyring v2 instance before using it.References
N/A
Checklist
Note
Medium Risk
Introduces new keyring migration and automatic v2 keyring creation logic that mutates
KeyringControllerstate and removes the legacy Snap keyring, which could impact account availability if migration edge-cases occur.Overview
Adds a concurrent-safe
SnapAccountService.migrate()that migrates accounts out of the legacy global Snap keyring into per-SnapKeyringType.Snap(v2) keyrings, then removes the legacy keyring entry.Updates
ensureReadyto always run migration first and to atomically create a v2 keyring for the requested Snap if missing, before waiting on Snap platform readiness. Adds@metamask/keyring-apidependency and expands tests to cover migration concurrency, account grouping-by-snap, and v2 keyring auto-creation behavior.Reviewed by Cursor Bugbot for commit 8452267. Bugbot is set up for automated code reviews on this repo. Configure here.